[charts] Remove `colors` prop from `SparkLineChart`. #16494

bernardobelchior · 2025-02-06T16:06:49Z

Remove colors prop from SparkLineChart.

In #16477, I deprecated the colors prop, but we can still do breaking changes for v8. This will reduce the maintenance burden slightly.

Also added codemod to migrate from the old colors prop to the new color prop.

I have followed (at least) the PR section of the contributing guide.

bernardobelchior · 2025-02-06T16:08:44Z

packages/x-codemod/src/v8.0.0/charts/rename-sparkline-colors-to-color/actual.spec.tsx

+      <SparkLineChart data={data} colors={['red']} />
+      <SparkLineChart data={data} colors={fn} />
+      <SparkLineChart data={data} colors={(mode) => (mode === 'light' ? ['black'] : ['white'])} />
+      <SparkLineChart data={data} colors="red" />


This is valid since colors can be an array of strings or a function, but added it to ensure that we handle invalid well (i.e., by not applying any transformation).

Removed this test case because TypeScript wasn't happy with invalid properties.

mui-bot · 2025-02-06T16:15:11Z

Deploy preview: https://deploy-preview-16494--material-ui-x.netlify.app/

Updated pages:

docs/data/migration/migration-charts-v7/migration-charts-v7.md

Generated by 🚫 dangerJS against cbc6e4e

codspeed-hq · 2025-02-06T16:17:49Z

CodSpeed Performance Report

Merging #16494 will not alter performance

_{Comparing bernardobelchior:remove-sparkline-colors (cbc6e4e) with master (721dbac)}

Summary

✅ 6 untouched benchmarks

bernardobelchior · 2025-02-06T16:14:52Z

packages/x-codemod/src/v8.0.0/charts/rename-sparkline-colors-to-color/expected.spec.tsx

+    (<React.Fragment>
+      <SparkLineChart data={data} color={['red'][0]} />
+      <SparkLineChart data={data} color={typeof fn === "function" ? mode => fn(mode)?.[0] : fn} />
+      <SparkLineChart data={data} color={mode => (mode => (mode === 'light' ? ['black'] : ['white']))(mode)?.[0]} />


This might seem a bit confusing, but what I'm doing is:

Given an arrow function declaration fnDecl, transform it into: mode => (fnDecl)(mode)?.[0].

We need the optional chaining operator because undefined is a valid value for colors, so we need be careful not to access [0] of an undefined value.

It's tricky and I think most of the users will prefer to just rewrite the function by themself.

bernardobelchior · 2025-02-06T16:16:53Z

packages/x-codemod/src/v8.0.0/charts/rename-sparkline-colors-to-color/expected.spec.tsx

+  return (
+    (<React.Fragment>
+      <SparkLineChart data={data} color={['red'][0]} />
+      <SparkLineChart data={data} color={typeof fn === "function" ? mode => fn(mode)?.[0] : fn} />


Not sure if this is a valid approach.

This introduces a slight runtime performance hit because, when codemodding, we can't guarantee whether fn is a function or an array.

If you think this or another example are too complex, I can just remove this and leave this as an unhandled case.

bernardobelchior · 2025-02-06T16:29:15Z

packages/x-codemod/src/v8.0.0/charts/rename-sparkline-colors-to-color/index.ts

+        // Don't know how to handle this case
+      }
+
+      // Only apply transformation if we know how to handle it


Is there any best practice for letting the user know where we caught an unhandled case? I know we aren't covering 100% of the cases, and some are easy to detect, but hard to fix. It would be nice if we could let the user know which ones have been detected to guide them towards where they need to do a manual fix.

I don't think we currently notify unhandled cases

You can propose that though 👍

Since it's run in the dev console, a console.warn should do the job

bernardobelchior · 2025-02-06T16:30:27Z

packages/x-codemod/src/v8.0.0/charts/rename-sparkline-colors-to-color/expected.spec.tsx

+  // prettier-ignore
+  return (
+    (<React.Fragment>
+      <SparkLineChart data={data} color={['red']?.[0]} />


I followed a "better safe than sorry" approach here. Adding the optional chain operator might save us from some crashes in edge cases. Happy to remove it if you think that's too much.

what will happen to var when var=['blue']?

github-actions · 2025-02-07T15:34:37Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

alexfauquette

It looks like you had a deep thaughts about the different ways users could mess with that breaking change :)

Why not for the two first cases in example. But the last one looks more confusing than helping.

Maybe adding a comment in the user codebase like

// codemod-message: We replaced `colors` by `color`. Make sure this props receives a string or a function returning a string.

It seems JSXAttributes have a leadingComments property for that

Or some console.warning could do the job too.

And in the changelog an instruction about how to run this specific codemod

alexfauquette · 2025-02-10T08:36:57Z

packages/x-codemod/src/v8.0.0/charts/rename-sparkline-colors-to-color/index.ts

+        // Don't know how to handle this case
+      }
+
+      // Only apply transformation if we know how to handle it


Since it's run in the dev console, a console.warn should do the job

alexfauquette · 2025-02-10T08:40:52Z

packages/x-codemod/src/v8.0.0/charts/rename-sparkline-colors-to-color/index.ts

+        colorAttributeExpression = j.chainExpression(
+          j.optionalMemberExpression(colorsAttributeExpression, j.literal(0)),
+        );


Why not

Suggested change

colorAttributeExpression = j.chainExpression(

j.optionalMemberExpression(colorsAttributeExpression, j.literal(0)),

);

colorAttributeExpression = colorsAttributeExpression.elements[0];

Which should do

- <SparkLineChart data={data} color={['red']} /> + <SparkLineChart data={data} color={'red'} />

Instead of

- <SparkLineChart data={data} color={['red']} /> + <SparkLineChart data={data} color={['red']?.[0]} />

Mostly for safety reasons, didn't want to change user's code too much, but maybe it's fine. I'm not able to come up with a common use case where it might break 🤔

To provide more information: if we use colorsAttributeExpression.elements[0], then weird cases like colors={[bool ? 'red' : 'blue']} will break. By doing ?.[0] they will still work, and the user can remove the ?.[0] if it isn't necessary.

Let's hope people did bool ? ['red'] : ['blue'] :)

More seriously, you can not write a codemod that is 100% safe. The level of abstraction of the AST is too low.

For me, the idea is to guess what users might have done and provide a solution.

By doing ?.[0] you end up in a situation where 100% of modified lines will work, but 100% modified lines will need to be modified by the user. (I don't see someone keeping this ?.[0].

The other solution does not guaranty 100% correctness, but

If it works, it's the natural solution

If it does not work, the line should be easy to find from the git diff

By the way why does it break? I tried with AST explorer, and it seems that it does the expected behavior

- colors={[test ? 'black' : 'white']} + color={test ? 'black' : 'white'}

https://astexplorer.net/#/gist/4cd2d7a6679b41c4f11c3c18f6259f6e/5c34b93e23c12aa7a992c7b1e815ffd4b8628f5a

Yeah, those are good points. I don't have a lot of codemods, so I'm still trying to understand where to do the trade-offs.

I was leaning more on being 100% correct even if the final code isn't as desirable, but I'm ok with opting for the simpler solution.

I'll simplify this 👍

By the way why does it break? I tried with AST explorer, and it seems that it does the expected behavior

Yeah, you're right. I'll update the code 👍

alexfauquette · 2025-02-10T08:52:25Z

packages/x-codemod/src/v8.0.0/charts/rename-sparkline-colors-to-color/expected.spec.tsx

+    (<React.Fragment>
+      <SparkLineChart data={data} color={['red'][0]} />
+      <SparkLineChart data={data} color={typeof fn === "function" ? mode => fn(mode)?.[0] : fn} />
+      <SparkLineChart data={data} color={mode => (mode => (mode === 'light' ? ['black'] : ['white']))(mode)?.[0]} />


It's tricky and I think most of the users will prefer to just rewrite the function by themself.

bernardobelchior · 2025-02-10T10:50:58Z

It looks like you had a deep thaughts about the different ways users could mess with that breaking change :)

Yeah, maybe too much 😛

Why not for the two first cases in example. But the last one looks more confusing than helping.

Maybe adding a comment in the user codebase like
// codemod-message: We replaced `colors` by `color`. Make sure this props receives a string or a function returning a string.

Yeah, that sounds like a good idea. I'll take a look into doing something like that.

If all messages start with // mui-x-codemod: it would be easy for a user to Ctrl+F to find the unhandled cases. If the user is using git or another VCS it would also be easy to catch in a code review/lint step. Seems like a great idea!

It seems JSXAttributes have a leadingComments property for that

Or some console.warning could do the job too.

Yeah, if we add any // mui-x-codemod: comment we could also console.warn to ensure the user knows that there were unhandled cases.

And in the changelog an instruction about how to run this specific codemod

👍

bernardobelchior · 2025-02-10T16:37:41Z

@alexfauquette due to adding the comment blocks we kind of lost the idempotency. I say kind of because it's the comments that broke the idempotency, however having more than one comment doesn't really break the code, so I'm not sure if it's a problem. If you run the codemod multiple times you'll get more comments than you should, but that doesn't break the code per se.

I can add it back, but it'll take me some time as I was having trouble finding previous element in jscodeshift. Apparently there's no API for that. Let me know if I should add it back or this is acceptable 👍

…o appease TypeScript.

alexfauquette

Looks good 👍

I added a commit to fix the TS CI. I hope it will pass

Having TS ok with the actual/expected is most of the time just impossible, because one is written to match previous API which by definition does not respect the typing of the new one

bernardobelchior commented Feb 6, 2025

View reviewed changes

bernardobelchior added breaking change component: charts This is the name of the generic UI component, not the React module! labels Feb 6, 2025

bernardobelchior commented Feb 6, 2025

View reviewed changes

bernardobelchior force-pushed the remove-sparkline-colors branch from 936f04b to e25f08c Compare February 7, 2025 08:50

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 7, 2025

bernardobelchior force-pushed the remove-sparkline-colors branch from e25f08c to 237df3b Compare February 7, 2025 16:07

bernardobelchior requested review from alexfauquette and JCQuintas February 7, 2025 16:07

bernardobelchior marked this pull request as ready for review February 7, 2025 16:07

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 7, 2025

alexfauquette reviewed Feb 10, 2025

View reviewed changes

bernardobelchior requested a review from alexfauquette February 10, 2025 16:37

bernardobelchior added 9 commits February 10, 2025 18:00

[charts] Remove colors prop from SparkLineChart.

437d360

Add test for unknown cases in codemod

58340bd

Remove usage of colors from docs

92ed432

Improvements to codemod

367952f

Disable no-shadow in codemod tests. Remove unhandled transformation t…

5a5d328

…o appease TypeScript.

Add codemod explanation.

7aca569

Leave comment for unhandled transformations.

9cb59f1

Simplify array access

633301d

Fix lint. Remove unwanted early return.

31ee2a7

bernardobelchior force-pushed the remove-sparkline-colors branch from 9f1c17d to 31ee2a7 Compare February 10, 2025 17:00

fix-ts

220ef6c

alexfauquette approved these changes Feb 11, 2025

View reviewed changes

bernardobelchior added 3 commits February 11, 2025 10:17

Update codemod readme

d4516eb

Add content to migration guide

9fbc5ce

Add more information regarding codemod

3f24292

alexfauquette approved these changes Feb 11, 2025

View reviewed changes

Fix lint

cbc6e4e

bernardobelchior merged commit e71e831 into mui:master Feb 11, 2025
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[charts] Remove `colors` prop from `SparkLineChart`. #16494

[charts] Remove `colors` prop from `SparkLineChart`. #16494

bernardobelchior commented Feb 6, 2025

bernardobelchior Feb 6, 2025

bernardobelchior Feb 7, 2025

mui-bot commented Feb 6, 2025 •

edited

Loading

codspeed-hq bot commented Feb 6, 2025 •

edited

Loading

bernardobelchior Feb 6, 2025

alexfauquette Feb 10, 2025

bernardobelchior Feb 6, 2025

bernardobelchior Feb 6, 2025

JCQuintas Feb 7, 2025

JCQuintas Feb 7, 2025

alexfauquette Feb 10, 2025

bernardobelchior Feb 6, 2025

JCQuintas Feb 7, 2025

github-actions bot commented Feb 7, 2025

alexfauquette left a comment

alexfauquette Feb 10, 2025

alexfauquette Feb 10, 2025

bernardobelchior Feb 10, 2025

bernardobelchior Feb 10, 2025

alexfauquette Feb 10, 2025

bernardobelchior Feb 10, 2025

alexfauquette Feb 10, 2025

bernardobelchior commented Feb 10, 2025

bernardobelchior commented Feb 10, 2025

alexfauquette left a comment •

edited

Loading

[charts] Remove colors prop from SparkLineChart. #16494

[charts] Remove colors prop from SparkLineChart. #16494

Conversation

bernardobelchior commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mui-bot commented Feb 6, 2025 • edited Loading

codspeed-hq bot commented Feb 6, 2025 • edited Loading

Merging #16494 will not alter performance

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 7, 2025

alexfauquette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernardobelchior commented Feb 10, 2025

bernardobelchior commented Feb 10, 2025

alexfauquette left a comment • edited Loading

Choose a reason for hiding this comment

[charts] Remove `colors` prop from `SparkLineChart`. #16494

[charts] Remove `colors` prop from `SparkLineChart`. #16494

mui-bot commented Feb 6, 2025 •

edited

Loading

codspeed-hq bot commented Feb 6, 2025 •

edited

Loading

alexfauquette left a comment •

edited

Loading